Fix mypy annotations in fermion_partitioning (#1282)#1349
Conversation
First slice for quantumlib#1282: correct generator return types and local variable annotations in measurements/fermion_partitioning.py. No runtime or API behavior changes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
mhucka
left a comment
mhucka
left a comment
There was a problem hiding this comment.
Thank you for doing this!
There is one change I would recommend. Although Any is valid, it is better to define so-called generic parameter types, along the following lines:
from typing import TypeVar
T = TypeVar('T')
def pair_within(labels: list[T]) -> Generator[tuple[T, ...], None, None]:
"""Generates pairings of labels that contain each pair at least once."""
...Applying this across other matching functions (like pair_between and pair_within_simultaneously) allows static analyzers to check that the output tuples contain elements of the exact same type as the input.
Would you be able to try to make the appropriate changes to this PR?
Replace Any with generic T and a Pairing type alias that matches runtime: tuples of (T, T) pairs (plus occasional bare T leftovers). Internal iterator combiners stay on Any. No behavior changes.
|
@mhucka reworked based on suggestion, mypy is clean on this file and all tests pass. Let me know how it looks, and thanks for the tip. <3 |
mhucka
left a comment
mhucka
left a comment
There was a problem hiding this comment.
I'm not done with the review on the update, but here's a start for a minor detail.
|
|
||
|
|
||
| def pair_within(labels: list) -> list: | ||
| def pair_within(labels: list[T]) -> Generator[Pairing, None, None]: |
There was a problem hiding this comment.
Here, the declaration should be changed to Pairing[T].
| def pair_within(labels: list[T]) -> Generator[Pairing, None, None]: | |
| def pair_within(labels: list[T]) -> Generator[Pairing[T], None, None]: |
Using Pairing without its type argument causes static type checkers like mypy to treat it as Pairing[Any], which often passes the type check, but does not give us the real benefits of the type declarations. The lack of the subscript T breaks the connection between the type T of the input list labels and the yielded element types.
|
|
||
|
|
||
| def pair_within_simultaneously(labels: list) -> tuple: | ||
| def pair_within_simultaneously(labels: list[T]) -> Generator[Pairing, None, None]: |
There was a problem hiding this comment.
| def pair_within_simultaneously(labels: list[T]) -> Generator[Pairing, None, None]: | |
| def pair_within_simultaneously(labels: list[T]) -> Generator[Pairing[T], None, None]: |
| def pair_within_simultaneously_binned(binned_majoranas: list) -> tuple: | ||
| def pair_within_simultaneously_binned( | ||
| binned_majoranas: list[list[T]], | ||
| ) -> Generator[Pairing, None, None]: |
There was a problem hiding this comment.
| ) -> Generator[Pairing, None, None]: | |
| ) -> Generator[Pairing[T], None, None]: |
| def pair_within_simultaneously_symmetric(num_fermions: int, num_symmetries: int) -> tuple: | ||
| def pair_within_simultaneously_symmetric( | ||
| num_fermions: int, num_symmetries: int | ||
| ) -> Generator[Pairing, None, None]: |
There was a problem hiding this comment.
This one is not a T but instead, an int:
| ) -> Generator[Pairing, None, None]: | |
| ) -> Generator[Pairing[int], None, None]: |
| generator_list = [_loop_iterator(pair_within, partition[j]) for j in range(len(partition))] | ||
| for dummy1 in range(len(partition[-2]) - 1 + len(partition[-2]) % 2): | ||
| pairing = tuple() | ||
| pairing: Pairing = () |
There was a problem hiding this comment.
| pairing: Pairing = () | |
| pairing: Pairing[T] = () |
| num_pairs = min(len(frag1), len(frag2)) | ||
|
|
||
| for index_offset in range(start_offset, num_iter): | ||
| pairing: Pairing |
There was a problem hiding this comment.
| pairing: Pairing | |
| pairing: Pairing[T] |
| def pair_between(frag1: list, frag2: list, start_offset: int = 0) -> tuple: | ||
| def pair_between( | ||
| frag1: list[T], frag2: list[T], start_offset: int = 0 | ||
| ) -> Generator[Pairing, None, None]: |
There was a problem hiding this comment.
| ) -> Generator[Pairing, None, None]: | |
| ) -> Generator[Pairing[T], None, None]: |
2 participants
First slice for #1282: correct generator return types and local variable annotations in measurements/fermion_partitioning.py. No runtime or API behavior changes.